Skip to content

Railcraft compat#384

Open
LonelyxiyaOVO wants to merge 11 commits intoCleanroomMC:masterfrom
LonelyxiyaOVO:Railcraft-compat
Open

Railcraft compat#384
LonelyxiyaOVO wants to merge 11 commits intoCleanroomMC:masterfrom
LonelyxiyaOVO:Railcraft-compat

Conversation

@LonelyxiyaOVO
Copy link
Copy Markdown

bro Railcraft compat

Copy link
Copy Markdown
Contributor

@Wizzerinus Wizzerinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a maintainer of this mod, just mentioning some things I noticed ;)

Comment thread src/main/java/com/cleanroommc/groovyscript/compat/mods/railcraft/CokeOven.java Outdated
Comment thread src/main/java/com/cleanroommc/groovyscript/compat/mods/railcraft/FluidFuels.java Outdated
Copy link
Copy Markdown
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few other things:

  1. instead of magic numbers as defaults, use the railcraft defaults directly - ie IRollingMachineCrafter.DEFAULT_PROCESS_TIME.
  2. run /gs generateWiki or enabling dev_generate_wiki in gradle.properties - check the log to ensure no warnings/missing lang keys.
  3. wizz also brought up great points, listen to those.

Comment thread gradle.properties
Comment thread dependencies.gradle
Comment thread examples/postInit/generated/railcraft_example.groovy Outdated

@Property(comp = @Comp(gte = 0))
private int time = 200;
private final List<IOutputEntry> outputs = new ArrayList<>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

annotate this with @Property. i would also suggest renaming this to output

Comment on lines +46 to +47
List<IRollingMachineCrafter.IRollingRecipe> recipes = new ArrayList<>(Crafters.rollingMachine().getRecipes());
recipes.removeIf(recipe -> recipe.getRegistryName().equals(r.getKey()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a new arraylist and then remove from that list? are you sure thats correct

List<IRollingMachineCrafter.IRollingRecipe> recipes = new ArrayList<>(Crafters.rollingMachine().getRecipes());
recipes.removeIf(recipe -> recipe.getRegistryName().equals(r.getKey()));
});
restoreFromBackup().forEach(r -> ModSupport.RAILCRAFT.get().rollingMachine.add(r.getKey(), r.getValue()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant do this, because RollingMachine#add runs #addScripted

Comment on lines +89 to +95
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
if (output.test(r.getRecipeOutput())) {
addBackup(Pair.of(r.getRegistryName(), r));
return true;
}
return false;
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can compact this significantly by using doAddBackup to this

Suggested change
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
if (output.test(r.getRecipeOutput())) {
addBackup(Pair.of(r.getRegistryName(), r));
return true;
}
return false;
});
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
return output.test(r.getRecipeOutput()) && doAddBackup(Pair.of(r.getRegistryName(), r));
});

which becomes

Suggested change
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
if (output.test(r.getRecipeOutput())) {
addBackup(Pair.of(r.getRegistryName(), r));
return true;
}
return false;
});
return Crafters.rollingMachine().getRecipes().removeIf(r -> output.test(r.getRecipeOutput()) && doAddBackup(Pair.of(r.getRegistryName(), r)));

import java.util.List;

@RegistryDescription
public class RollingMachine extends VirtualizedRegistry<Pair<ResourceLocation, IRollingMachineCrafter.IRollingRecipe>> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IRollingMachineCrafter.IRollingRecipe stores the ResourceLocation, so theres no need to have it be Pair<ResourceLocation, IRollingMachineCrafter.IRollingRecipe> instead of just IRollingMachineCrafter.IRollingRecipe

Comment on lines +33 to +46
if (time <= 0) {
GroovyLog.msg("Error adding Railcraft Blast Furnace recipe")
.error()
.add("time must be greater than 0, got: {}", time)
.post();
return null;
}
if (slag < 0) {
GroovyLog.msg("Error adding Railcraft Blast Furnace recipe")
.error()
.add("slag must be non-negative, got: {}", slag)
.post();
return null;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate in the builder

Comment on lines +141 to +148
if (time <= 0) {
msg.add("time must be greater than 0, got: {}", time);
time = IBlastFurnaceCrafter.SMELT_TIME;
}
if (slag < 0) {
msg.add("slag must be non-negative, got: {}", slag);
slag = 1;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are any messages, the validation fails. just do

Suggested change
if (time <= 0) {
msg.add("time must be greater than 0, got: {}", time);
time = IBlastFurnaceCrafter.SMELT_TIME;
}
if (slag < 0) {
msg.add("slag must be non-negative, got: {}", slag);
slag = 1;
}
msg.add(time <= 0, "time must be greater than 0, got: {}", time);
msg.add(slag < 0, "slag must be non-negative, got: {}", slag);

Comment on lines +116 to +117
@Property
private FluidStack fluid;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this, use fluidOutput from parent

@RegistryDescription
public class FluidFuels extends VirtualizedRegistry<FluidFuels.FuelEntry> {

private final Map<String, Integer> backupFuels = new HashMap<>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now completely unused


public boolean remove(IRollingMachineCrafter.IRollingRecipe recipe) {
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
if (r == recipe) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if r == recipe well you can just do addBackup(recipe) + Crafters.rollingMachine().getRecipes().remove(recipe) then

Comment thread buildscript.properties Outdated
# Version of your mod.
# This field can be left empty if you want your mod's version to be determined by the latest git tag instead.
modVersion = 1.4.3
modVersion = 1.4.4
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo changing the version

@LonelyxiyaOVO
Copy link
Copy Markdown
Author

I've fixed it. Please take a look and let me know if there are any issues

Copy link
Copy Markdown
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one new thing. please actually resolve the other issues you have marked as resolved, but have not resolved.

Comment thread src/main/java/com/cleanroommc/groovyscript/compat/mods/railcraft/CokeOven.java Outdated
@LonelyxiyaOVO
Copy link
Copy Markdown
Author

check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants